Restore recipe parameters functionality#3530
Conversation
| "format": "double", | ||
| "description": "Cost per token for output (optional)", | ||
| "nullable": true | ||
| }, |
There was a problem hiding this comment.
upstream changes can ignore
| * Cost per token for output (optional) | ||
| */ | ||
| output_token_cost?: number | null; | ||
| /** |
There was a problem hiding this comment.
upstream changes can ignore
There was a problem hiding this comment.
should we maybe have something in CI that checks whether regenerating the openapi.json is a no-op?
There was a problem hiding this comment.
Not sure if ci can help this other than maybe flagging when someone didn't manually generate it? It should be manually generated whenever someone makes a related change. I run the full npm run start locally which generates the types so occasionally I run into this issue where someone didn't generate the types upstream.
There was a problem hiding this comment.
yeah, I meant we could run the typegen in ci and compare if it changes anything and if it does, tell the user they forgot to run it but did make a change
There was a problem hiding this comment.
Ah gotcha, good idea! Will follow up with that 👍
| )} | ||
| </div> | ||
| {/* Input row with inline action buttons wrapped in form */} | ||
| <form onSubmit={onFormSubmit} className="relative flex items-end"> |
There was a problem hiding this comment.
form now wraps only what it should, the input and the submit/action buttons, before it was wrapping all the actions under the chat input causing the chat form to submit from descendant buttons in launched modals or the working directory click
| {error.message || 'Honk! Goose experienced an error while responding'} | ||
| </div> | ||
|
|
||
| {/* Expandable Error Details */} |
There was a problem hiding this comment.
Removed this expandable error details area as it wasn't providing any other useful context than what is displayed on the screen.
DOsinga
left a comment
There was a problem hiding this comment.
Thanks for jumping on this. I added some maybe too detailed review for a quick fix. In general my question is also, what can we do to test this automatically? these flows are hard to follow I find and seem like having bugs in them would be easy
| * Cost per token for output (optional) | ||
| */ | ||
| output_token_cost?: number | null; | ||
| /** |
There was a problem hiding this comment.
should we maybe have something in CI that checks whether regenerating the openapi.json is a no-op?
| if (recipeConfig?.title && recipeConfig.title !== currentRecipeTitle) { | ||
| // Only reset usage if we're switching between different recipes | ||
| // Don't reset if we're going from no recipe to a recipe (initial load) | ||
| // or if we already have messages (ongoing conversation) |
There was a problem hiding this comment.
I think we have this comment because the code is hard to follow; let's see if we can fix that and drop the comment instead
There was a problem hiding this comment.
Good call refactored and simplified
| // Reset recipe usage tracking when recipe changes | ||
| useEffect(() => { | ||
| if (recipeConfig?.title !== currentRecipeTitle) { | ||
| const previousTitle = currentRecipeTitle; |
There was a problem hiding this comment.
suggest to move this outside the if, so the comparisons are easier to follow
| // Only reset usage if we're switching between different recipes | ||
| // Don't reset if we're going from no recipe to a recipe (initial load) | ||
| // or if we already have messages (ongoing conversation) | ||
| if (previousTitle && recipeConfig?.title && previousTitle !== recipeConfig.title) { |
There was a problem hiding this comment.
here and below, we already checked previousTitle != recipeConfig.title so that bit is always going to be true. So for this to not be true, either previousTitle or recipeConfig?title needs to be falsey. if that's the case we should check on that
| parameters.forEach((param) => { | ||
| if (param.default) { | ||
| initialValues[param.key] = param.default; | ||
| // Set default value for optional parameters if they have a default value |
There was a problem hiding this comment.
I don't think we need the comment here
| if (!allParams.some((param) => param.key === promptParam.key)) { | ||
| allParams.push(promptParam); | ||
| } | ||
| }); |
There was a problem hiding this comment.
this is O(n^2) - consider using a map to do this more efficiently
| const formattedParam: Parameter = { | ||
| key: param.key, | ||
| input_type: 'string', | ||
| input_type: param.input_type || 'string', // Use actual input_type instead of hardcoded 'string' |
|
|
||
| // Pre-fill input with recipe prompt instead of auto-sending it | ||
| const initialPrompt = useMemo(() => { | ||
| // Don't show initial prompt if we already have messages (ongoing conversation) |
There was a problem hiding this comment.
are we calling this in the wrong place? the initial prompt shouldn't change because we have a conversation, we should just not put at as the non-initial prompt
| setRecipeParameters(inputValues); | ||
| // Store parameters in chat context instead of local state | ||
| if (chatContext?.setRecipeParameters) { | ||
| chatContext.setRecipeParameters(inputValues); |
There was a problem hiding this comment.
in what case is chatContext undefined here? can we use the type system to make sure it is not?
There was a problem hiding this comment.
its a provider that we only use for certain cases at runtime so the type system isn't enough to catch it unfortunately
| | string; // Can also be a string representation | ||
| // Properties added for scheduled execution | ||
| scheduledJobId?: string; | ||
| isScheduledExecution?: boolean; |
There was a problem hiding this comment.
Not for now, but we have this definition also on the server. for some reason we're not using the openapi mechanism to do recipe calls, so we duplicate the definition here. do you think we can fix that?
There was a problem hiding this comment.
Good call, went ahead and added it to the openai types
|
@DOsinga great review thanks! All issues addressed pls take another look. Regarding how we can verify, unfortunately until there are more functional/ui tests covering this process its a manual testing process. I'll make sure we cover recipes in the tests as we build them out. |
ui/desktop/src/components/pair.tsx
Outdated
| messageHistoryIndex: number; | ||
| messages: Message[]; | ||
| recipeConfig?: Recipe | null; // Add recipe configuration to chat state | ||
| recipeParameters?: Record<string, string> | null; // Add recipe parameters to chat state |
There was a problem hiding this comment.
why are hub and pair having the same diff & code here?
There was a problem hiding this comment.
Good catch also, refactored to use a shared ChatType instead 👍
|
hmm, weird error. can you try merging in main? |
…ters * 'main' of github.com:block/goose: Add recipe install warning (#3537) Replace mcp_core::resource::* with rmcp types (#3563) Add YouTube video embed to using-goosehints.md (#3560) fix: ensure retry-config and success-criteria are populated in openapi spec (#3575) fix: use sequential when sub recipe task is 1. (#3573) fix: track message id to keep like with like (#3572) Replace mcp_core::prompt with rmcp::model types (#3561) feat (ui): close recipe modals with esc key (#3568) feat: recipes can retry with success criteria (#3474) Env var to set Ollama request timeout (#3516)
…zane/recipe-parameters * 'zane/recipe-parameters' of github.com:block/goose: Add recipe install warning (#3537) Replace mcp_core::resource::* with rmcp types (#3563) Add YouTube video embed to using-goosehints.md (#3560) fix: ensure retry-config and success-criteria are populated in openapi spec (#3575) fix: use sequential when sub recipe task is 1. (#3573) fix: track message id to keep like with like (#3572) Replace mcp_core::prompt with rmcp::model types (#3561) feat (ui): close recipe modals with esc key (#3568) feat: recipes can retry with success criteria (#3474) Env var to set Ollama request timeout (#3516)
* 'main' of github.com:block/goose: (23 commits) fix: add fallback id to messages if none provided (#3584) feat: migrate ErrorData from internal mcp crates to rmcp version (#3586) fix: adjust subrecipe description to allow running tests (#3585) Scenario tests (#3430) feat: migrate JsonRpcMessage/Request/Response/Error/Notification from internal mcp crates to rmcp versions (#3564) Restore recipe parameters functionality (#3530) feat: Enhanced loading states with thinking icons and flying bird animation (#3478) Agent loop defensive (#3554) chore: remove needless clone() in goose/providers (#2528) Add recipe install warning (#3537) Replace mcp_core::resource::* with rmcp types (#3563) Add YouTube video embed to using-goosehints.md (#3560) fix: ensure retry-config and success-criteria are populated in openapi spec (#3575) fix: use sequential when sub recipe task is 1. (#3573) fix: track message id to keep like with like (#3572) Replace mcp_core::prompt with rmcp::model types (#3561) feat (ui): close recipe modals with esc key (#3568) feat: recipes can retry with success criteria (#3474) Env var to set Ollama request timeout (#3516) Updating docs to match new UI (#3552) ...
* main: docs: desktop recipe format (#3594) Fix model display name not being updated immediately after leaving settings (#3587) Added option to summarize the chat when an error is triggered (#3598) Remove mcp_macros and unused types (#3581) fix: add fallback id to messages if none provided (#3584) feat: migrate ErrorData from internal mcp crates to rmcp version (#3586) fix: adjust subrecipe description to allow running tests (#3585) Scenario tests (#3430) feat: migrate JsonRpcMessage/Request/Response/Error/Notification from internal mcp crates to rmcp versions (#3564) Restore recipe parameters functionality (#3530) feat: Enhanced loading states with thinking icons and flying bird animation (#3478) Agent loop defensive (#3554) chore: remove needless clone() in goose/providers (#2528) Add recipe install warning (#3537) Replace mcp_core::resource::* with rmcp types (#3563) Add YouTube video embed to using-goosehints.md (#3560) fix: ensure retry-config and success-criteria are populated in openapi spec (#3575)
* main: feat: subagent turn count, simple agent loop (#3597) feat: subagent independent extension manager (#3596) Improve session history loading resiliency (#3588) Added logging and changed default route case to not redirect to home when there is an invalid route (#3610) fix: chat sidebar layout overlapping content occasionally (#3590) fix: loading shared sessions (#3607) docs: use installer component for tutorial, add updating extensions section (#3608) fix: show token alert popover during agent responses and agent failure cases (#3536) reuse the cancellation token in the agent level (#3599) Docs: Move MongoDB tutorial to MCP section (#3602) docs: desktop recipe format (#3594) Fix model display name not being updated immediately after leaving settings (#3587) Added option to summarize the chat when an error is triggered (#3598) Remove mcp_macros and unused types (#3581) fix: add fallback id to messages if none provided (#3584) feat: migrate ErrorData from internal mcp crates to rmcp version (#3586) fix: adjust subrecipe description to allow running tests (#3585) Scenario tests (#3430) feat: migrate JsonRpcMessage/Request/Response/Error/Notification from internal mcp crates to rmcp versions (#3564) Restore recipe parameters functionality (#3530)
Signed-off-by: Adam Tarantino <[email protected]>
Recipe parameters stopped working after the new ui launch.
This adds them back and fixes some issues I noticed along the way:
Bonus:
select,booleanandnumber.